Set up server side row model for table visualization #12272
Conversation
🧪 Storybook is successfully deployed!📊 Dashboard:
|
marthasharkey
requested review from
jdunkerley,
radeusgd,
GregoryTravis,
AdRiley,
Frizi,
farmaazon,
vitvakatu and
kazcw
as code owners
| executeExpression: ( | ||
| visulizationModule: string, | ||
| expressionString: string, | ||
| formatFunction: | ||
| | ((arg: any, tempModule: Ast.MutableModule) => Ast.Owned<Ast.MutableExpression>) | ||
| | null, | ||
| ...positionalArgumentsExpressions: string[] | ||
| ) => | ||
| executeExpression( | ||
| visulizationModule, | ||
| expressionString, | ||
| formatFunction, | ||
| ...positionalArgumentsExpressions, | ||
| ), |
| function setRowLimit(newRowLimit: number) { | ||
| if (newRowLimit !== rowLimit.value) { | ||
| rowLimit.value = newRowLimit | ||
| config.setPreprocessor( | ||
| 'Standard.Visualization.Table.Visualization', | ||
| 'prepare_visualization', | ||
| newRowLimit.toString(), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
The Vue pattern for this is to set rowLimit.value directly, and run the side-effect as needed via a watcher, like watchEffect(() => config.setPreprocessor('Standard.Visualization.Table.Visualization', 'prepare_visualization', rowLimit.value.toString())).
| @@ -92,6 +95,50 @@ export function useVisualizationData({ | |||
| }, | |||
| ) | |||
|
|
|||
| const executeExpression = async ( | |||
There was a problem hiding this comment.
Since this function is part of the API for visualizations, its signature should be as simple as possible, and shouldn't contain any any types. I think it could be generalized to: executeExpression(expression: (nodeOutputIdentifier: string) => string): Promise<Result<unknown> | null>. The logic for constructing the expression (L116-L140) can be left to the caller.
There was a problem hiding this comment.
Yeah I did think it had started to get too specific for this usecase, thank you
| all_rows = ["all_rows_count", all_rows_count] | ||
| has_index_col = ["has_index_col", include_index_col] | ||
| has_index_col = ["has_index_col", True] | ||
| is_ssrm = if is_db_table then False else all_rows_count > max_rows |
There was a problem hiding this comment.
IMHO it may be better to spell out what SSRM is here as it is not that obvious IMO when looking at the code.
There was a problem hiding this comment.
yes I agree, ssrm is a ag grid term so I'll change it to be more descriptive
| applies the sort model to the table. | ||
|
|
||
| Arguments: | ||
| - start_number: the start index of the next 100 rows to get | ||
| - sort_col_index_list: indexes of columns with sorts applies | ||
| - sort_direction_list: list of sort directions 1 for ascending -1 for descending | ||
|
|
||
| get_distinct_values_for_column : Table -> Integer -> JS_Object | ||
| get_distinct_values_for_column x column_index = |
There was a problem hiding this comment.
The doc comment is not aligned with the method that it is documenting, perhaps copied from apply_sort_to_table.
| Arguments: | ||
| - table: table to sort | ||
| - filter_col: indexes of columns with sorts applies | ||
| - filter_action: list of filter actions |
There was a problem hiding this comment.
I'm a bit confused - the comment here says that this is a list of actions but the type signature says Filter_Condition - indicating a single value not a list. Looking at the code it is not a list, so perhaps this comment became outdated?
| result = case filter_action of | ||
| Filter_Condition.Between _ _ _ -> table.filter (filter_col+1) (Filter_Condition.Between filter_val filter_to_val) | ||
| Filter_Condition.Is_In _ _ -> if filter_val.length > 0 then table.filter (filter_col+1) (Filter_Condition.Is_In filter_val) else table.filter filter_col (Filter_Condition.Is_In [""]) | ||
| _ -> table.filter (filter_col+1) (filter_action (filter_val)) | ||
| result |
There was a problem hiding this comment.
very minor nitpick but the result is assigned just to be returned, isn't it better to just remove result = and return the case of directly?
This should have equivalent behaviour:
| result = case filter_action of | |
| Filter_Condition.Between _ _ _ -> table.filter (filter_col+1) (Filter_Condition.Between filter_val filter_to_val) | |
| Filter_Condition.Is_In _ _ -> if filter_val.length > 0 then table.filter (filter_col+1) (Filter_Condition.Is_In filter_val) else table.filter filter_col (Filter_Condition.Is_In [""]) | |
| _ -> table.filter (filter_col+1) (filter_action (filter_val)) | |
| result | |
| case filter_action of | |
| Filter_Condition.Between _ _ _ -> table.filter (filter_col+1) (Filter_Condition.Between filter_val filter_to_val) | |
| Filter_Condition.Is_In _ _ -> if filter_val.length > 0 then table.filter (filter_col+1) (Filter_Condition.Is_In filter_val) else table.filter filter_col (Filter_Condition.Is_In [""]) | |
| _ -> table.filter (filter_col+1) (filter_action (filter_val)) |
There was a problem hiding this comment.
yes I think I potentially has this while writing the function and forgot to go back and simplify
| get_rows_for_table x start_number sort_col_index_list=Nothing sort_direction_list=Nothing filter_col=Nothing filter_action=Nothing filter_val=Nothing filter_extra_val=Nothing = | ||
| max_rows=100 | ||
| col_name = x.make_temp_column_name |
There was a problem hiding this comment.
maybe a nitpick but why sometimes use table for a table argument and sometimes just x?
I think we should consistently use table in these functions. It makes the code clearer to read.
There was a problem hiding this comment.
yes I agree, I thought I had updates all to table but must have missed this one!!
Pull Request Description
Introduces the server side row model to appropriate table visualizations.
Creates an extra get_rows_for_table method that gets the row data rather than this being serialised in the existing prepare visualization step.
Adds method to visualisationConfig to allow the visualization to execute and expression to access this method.
prepare_vizualization still gets column definition information and largely remains the same
get_rows_for_table sorts and filters the table and returns the correct rows for the visualization.
TableViz behaviour remains mostly the same but filtering/soring large tables is no longer blocked:
Important Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
or the Snowflake database integration, a run of the Extra Tests has been scheduled.